fix(static): send the request once when retries is below 1 - #420
Merged
D4Vinci merged 2 commits intoAug 23, 2026
Conversation
`_make_request` looped `for attempt in range(max_retries)`, so a `retries`
value of 0 or below skipped the request entirely and fell through to
`raise RuntimeError("No active session available.")` while the session was
alive. `retries=None` - type-legal through `RequestsSession`,
`GetRequestParams` and `FetcherSession.__init__` - failed one step earlier
with a `TypeError` raised by `range(None)`.
- Clamp `max_retries` to at least one attempt in both the sync and the async
`_make_request`, so those values send the request once without retrying.
The clamped value also feeds the `attempt < max_retries - 1` check and the
"Failed after N attempts" log, so they stay consistent.
- Add regression tests for session-level and per-request `retries` of 0, -1
and None, sync and async, plus the public `Fetcher`/`AsyncFetcher` path.
Contributor
Author
|
Small update rather than a ping: after await ScraplingMCPServer.make_request("http://127.0.0.1:1/", retries=0)
# RuntimeError: No active session available.The diff here is unchanged and still merges cleanly onto Disclosure: this comment was written with AI assistance (Claude), per |
Owner
|
If that happened, then it would be the user doing that for themselves, but anyway, let's fix it. Thanks @Yigtwxx |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Both
_make_requestimplementations inscrapling/engines/static.pydrive the retry loop straight off theretriesvalue:With
retries=0the loop body never runs, so no HTTP request is made at all and control falls through to that trailingraise. The message is also wrong: the session is alive, it was simply never used.A negative value behaves the same.
retries=Nonefails one step earlier, onrange(None):Noneis worth handling because it is type-legal on three public surfaces:RequestsSession/GetRequestParams(scrapling/engines/_browsers/_types.py),FetcherSession.__init__(retries: Optional[int] = 3), and_shell_signatures.py. Every entry point is affected —Fetcher,AsyncFetcher,FetcherSession(retries=0)andAsyncFetcherSession.The browser engines never had this problem, because they declare the same parameter as a bounded type and reject the value cleanly:
The HTTP side has no such bound, and the MCP layer re-exposes the parameter unbounded —
ScraplingMCPServer.getandbulk_getboth declareretries: Optional[int] = 3and forward the value unconditionally. An LLM reading "Number of retry attempts. Defaults to 3." and passing0to mean "do not retry" gets the bogusRuntimeErrorinstead of a page:The change
max_retriesis clamped to at least one attempt at the single place each method reads it:or 1mapsNoneand0to1, and the enclosingmax(1, ...)covers negatives —max(1, None)would raise on its own, which would leave theNonepath broken.I went with clamping rather than mirroring
RetriesCount'sge=1on the HTTP side, becauseretries=0is a value users reasonably pass today meaning "send it once, do not retry", and turning that into a hard failure is a breaking change for a bug fix. It also matches themax_pagesclamp in #393.Since
max_retriesis the single variable feeding the loop, theattempt < max_retries - 1check and thef"Failed after {max_retries} attempts"log, all three stay consistent. Retry behaviour forretries >= 1is untouched, which the existingtest_proxy_rotates_per_retry_attempttests still assert.The two trailing
raise RuntimeError("No active session available.")lines stay as they are. They become genuinely unreachable, butmypyreportsMissing return statementon both methods without a terminalraiseafter the loop, so removing them would mean a larger diff for no behavioural gain.Tests
Ten cases added across the four existing fetcher test files, none of which touch the public internet:
tests/fetchers/{sync,async}/test_requests_session.py: session-levelretriesof0,-1andNone, plus a per-requestretries=0overriding a session default of3. These patchcurl_cffi'srequestand assertcall_count == 1, which is stronger than "no exception was raised" — it also catches a fix that loops more than once.tests/fetchers/{sync,async}/test_requests.py:Fetcher.get(..., retries=0)andretries=-1againstpytest_httpbin. This covers the other branch of_make_request— the one-off session thatFetcherClientcreates — and proves a request actually goes out.All ten fail on
devand pass with the change.pytest tests/fetchersis green (241 passed), andruff check,ruff format --check,mypy,pyright,banditandvermin -t=3.10-are all clean locally on Python 3.12.Type of change:
Additional information
I deliberately left the docstrings alone. After the clamp,
":param retries: Number of retry attempts. Defaults to 3."is not wrong for any input, and that exact sentence exists in eleven places (eight method docstrings instatic.py,FetcherSession.__init__, and the two MCP tools) — editing a subset would be inconsistent, and editing all eleven would bury a two-line behaviour fix. Happy to add a sentence about values below 1 if you would rather have it spelled out.Checklist:
AI assistance disclosure
Per AI_POLICY.md: I used Claude Code while working on this. It helped me compare how the HTTP fetchers handle
retriesagainst the browser validators'RetriesCount, and it drafted the patch and the tests. I found and reproduced the defect myself, decided on clamping over rejecting, checked that no existing test or doc pinned the old behaviour, reviewed the diff, and ran the test suite and the quality checks locally.